Conversation
- 이동거리로 주입해야할 연료량을 계산하는 로직 구현
- generateChargeQuantityByName()
- 자동차_이름과_연료량을_받아_map을_반환한다()
- List<String>으로 반환
- 이름엔 공백이 들어올 수 없다
- 중복된 이름은 들어올 수 없다
- 게임플레이어가 카드를 더 받을 수 있는 조건은 일반플레이어와 딜러플레이어가 다르다 - 게임플리어와 딜러플레이어의 최종승패 출력 방식은 다르다
- ACE가 포함된것과 포함되지 않은 것 모두 정상적으로 작동하는지 검증
Rok93
left a comment
Rok93
left a comment
There was a problem hiding this comment.
다미님 안녕하세요! 블랙잭 미션을 함께하게 된 김경록입니다. 🙇♂️
블랙잭 미션 구현 잘해주셨네요! 👍👍
몇몇 코멘트 남겨두었으니 확인해서 반영해주세요! 🙏
이해가 잘 안되거나 어려운 부분은 언제든지 DM 주시면 됩니다. 😉
src/main/java/rentcar/domain/K5.java
Outdated
| @Override | ||
| double getTripDistance() { | ||
| return this.distance; | ||
| } |
There was a problem hiding this comment.
다른 Car 클래스의 하위 구현체(K5, Avante, Sonata)들 모두 해당 메서드를 완전 동일하게 재정의하고 있어요. 🤔
해당 메서드는 상위 클래스(= Car)에 두는 것이 좋지 않을까요? 😃
There was a problem hiding this comment.
추가로 재정의한 메서드들은 모두 접근 제한자가 default군요?
적절한 접근 제한자로 변경해보면 어떨까요? 🤗
There was a problem hiding this comment.
같은 패키지 안에서 사용하기에 default로 접근 제한자를 변경하였는데요! public으로 수정하는 편이 좋을까요? 🤔
| public Map<String, Double> generateChargeQuantityByName() { | ||
| Map<String, Double> map = new HashMap<>(); |
There was a problem hiding this comment.
아래와 같이 변경해보면 어떨까요? 😃
| public Map<String, Double> generateChargeQuantityByName() { | |
| Map<String, Double> map = new HashMap<>(); | |
| public Map<Car, Double> generateChargeQuantityByName() { | |
| Map<Car, Double> result = new HashMap<>(); |
|
|
||
| @DisplayName("이동거리로 주입해야할 연료량을 계산한다.") | ||
| @Test | ||
| public void getChargeQuantity() { |
There was a problem hiding this comment.
Parameterized Test를 참고해서, 테스트를 리팩토링해보면 좋을 것 같아요. 😃
(@MethodSource를 이용해보면 더 효율적인 코드로 변경할 수 있을 것 같아요. 🤗)
|
|
||
| RentCompany rentCompany = new RentCompany(cars); | ||
|
|
||
| Map<String, Double> chargeQuantityByName = rentCompany.generateChargeQuantityByName(); |
There was a problem hiding this comment.
개인적인 팁인데, 테스트 케이스에서 검증하고자 하는 결과 값은 result 혹은 actual와 같은 변수명을 쓰면 네이밍 걱정을 덜 할 수 있어요! 😃
| public GamePlayer getDealer() { | ||
| return players.get(0); | ||
| } | ||
|
|
||
| public List<GamePlayer> getPlayers() { | ||
| return Collections.unmodifiableList(players.subList(1, players.size())); | ||
| } |
There was a problem hiding this comment.
만약 GamePlayers의 상태 값(=List )의 순서가 변경되면 어떻게 될까요? 🤔
의도한대로 동작하지 않을거에요. 😱
지금의 코드는 순서에 의존적인 코드라고 볼 수 있어요. 👀
순서에 의존하지 않는 코드로 변경해볼 수는 없을까요? 😃
| import blackjack.view.OutputView; | ||
| import java.util.List; | ||
|
|
||
| public class Dealer { |
There was a problem hiding this comment.
Dealer와 DealerPlayer는 어떤 차이점이 있나요? 🤔
사실상 네이밍 만으로는 명확하게 구분할 수 없을 거예요. 👀
Dealer 클래스의 함수들을 살펴보니 사실상 BlackjackGame 객체 역할을 하는 것 같아요. 🧐
| } | ||
|
|
||
| public void initializeGame(final GamePlayers gamePlayers) { | ||
| List<GamePlayer> players = gamePlayers.getAllPlayers(); |
There was a problem hiding this comment.
gamePlayers 객체에게 메시지를 보내보면 어떨까요? 🤔
아래 playGame 메서드의 경우에도 객체의 상태 값을 외부로 꺼내서 직접 로직을 수행하는 로직들이 보이네요. 👀
객체에게 메시지를 보내서 결과만 받아볼 수 있도록 구조를 변경해보면 좋을 것 같아요. 🤗
| } | ||
|
|
||
| private void playerGameProcess(final GamePlayer player) { | ||
| while (player.isContinue() && InputView.getPlayerChoice(player)) { |
There was a problem hiding this comment.
Dealer 클래스는 Model 객체인 것 같은데 해당 로직에 View 로직들이 존재하고 있어요. 🥲
MVC 패턴에 따라 View와 Model을 분리해주세요! 🙏
| System.out.println(RESULT_HEADER_LOG); | ||
| List<GamePlayer> players = gamePlayers.getAllPlayers(); | ||
| for (GamePlayer player : players) { | ||
| System.out.println(String.format(RESULT_GAME_LOG, player.getName(), player.getGameResult(gamePlayers))); |
There was a problem hiding this comment.
player#getGameResult 메서드는 비즈니스 로직인 것 같아요. 👀
OutputView에 비즈니스 로직이 있으면 안 될 것 같아요. 🙄
- this.DISTANCE_PER_LITER > DISTANCE_PER_LITER
- createWithShuffling > create
- getAceCount > countAceCard
- getBestSumWithAce > calculateBestSumWithAce
3 participants
안녕하세요 리뷰어님! 연료 주입, 블랙잭 미션 step1 제출합니다.😀
이번 미션은 성현님과 함께 구조적으로 많은 고민을 하며 설계를 하고 또 리팩터링을 진행하였습니다.
테스트 코드, 설계 측면에서 많은 피드백 부탁드립니다🙌
감사합니다🙂